Descriptor size cannot be negative #1285

sudo-bmitch · 2025-09-05T18:25:32Z

This is in response to #153 (comment). I didn't say "MUST be positive" because we've seen some cases where a zero length blob has been permitted.

I'm not a JSON schema expert, but it passes the tests. Other changes to descriptor_test.go were for consistency.

Signed-off-by: Brandon Mitchell <[email protected]>

tianon

Thanks for considering the zero size edge case I forgot about (frankly I forgot that "MUST be positive" is ambiguous about whether zero is positive 😂).

tianon · 2025-09-05T18:27:42Z

(fwiw, I was quoting #153 (comment))

sudo-bmitch · 2025-09-05T18:28:43Z

Thanks for considering the zero size edge case I forgot about (frankly I forgot that "MUST be positive" is ambiguous about whether zero is positive 😂).

As long as we don't get into negative zero scenarios. 😂

tianon · 2025-09-05T18:29:39Z

"MUST NOT be negative"!!! ✊

cyphar · 2025-09-06T04:14:16Z

Makes sense, especially given how critical sizes are to avoiding DoS attacks.

VerifiedReadCloser previously would allow for negative ExpectedSize to disable the size checking features added in commit ad66299 ("pkg: hardening: expand to verify descriptor length"). This was based on a somewhat overly-permissive reading of the discussion in opencontainers/image-spec#153 which was finally clarified in opencontainers/image-spec#1285. Unknown sizes are a classic DoS vector, so allowing them seems like a bad idea in general. Signed-off-by: Aleksa Sarai <[email protected]>

This was implicitly allowed by VerifiedReadCloser, and while we have closed that hole it's probably best to provide a more helpful error message. Ref: opencontainers/image-spec#1285 Signed-off-by: Aleksa Sarai <[email protected]>

This was implicitly allowed by VerifiedReadCloser, and while we have closed that hole it's probably best to provide a more helpful error message. Not blocking this earlier was mostly due to a somewhat overly-permissive reading of the discussion in opencontainers/image-spec#153 which was finally clarified in opencontainers/image-spec#1285. Unknown sizes are a classic DoS vector, so allowing them (especially for descriptors where it makes little sense to have an unknown size) seems like a bad idea in general. Ref: opencontainers/image-spec#1285 Signed-off-by: Aleksa Sarai <[email protected]>

VerifiedReadCloser previously would allow for negative ExpectedSize to disable the size checking features added in commit ad66299 ("pkg: hardening: expand to verify descriptor length"). This was added partially because a somewhat overly-permissive reading of the discussion in opencontainers/image-spec#153 (which was finally clarified in opencontainers/image-spec#1285), but was also necessary for some users of VerifiedReadCloser that did not really know the proper blob size. We have now adjusted all of those callers, so there is no longer any reason to continue supporting this. Unknown sizes are a classic DoS vector, so allowing them seems like a bad idea in general. We might need to adjust this if/when umoci grows OCI distribution-spec support, but for now it isn't needed. Signed-off-by: Aleksa Sarai <[email protected]>